Rebuild compilations that use top-level statements #51845

RikkiGibson · 2021-03-12T19:34:23Z

Addresses the following error

Verifying C:\Users\rikki\src\roslyn\artifacts\obj\CompilersIOperationGenerator\Release\net5.0\IOperationGenerator.dll with pdb [embedded]

  Diagnostics
    C:\Users\rikki\src\roslyn\src\Tools\Source\CompilerGeneratorTools\Source\IOperationGenerator\Program.cs(11,1): error CS1558: '<Program>$' does not have a suitable static 'Main' method
    error CS8804: Cannot specify /main if there is a compilation unit with top-level statements.

cston · 2021-03-12T23:21:11Z

src/Compilers/Core/RebuildTest/CSharpRebuildTests.cs

+using System.IO;
+using System.Threading;
+using Microsoft.CodeAnalysis.VisualBasic;
+using System.Text;


Are all of these usings needed? #Closed

cston · 2021-03-12T23:24:51Z

src/Compilers/Core/RebuildTest/CSharpRebuildTests.cs

+ return new ResolvedSource(OnDiskPath: null, text, new SourceFileInfo(path, text.ChecksumAlgorithm, text.GetChecksum().ToArray(), text, embeddedCompressedHash: null));
+ }).ToImmutableArray();
+ var references = original.References.ToImmutableArray();
+ var (compilation, isError) = bc.CreateCompilation(optionsReader, path, sources, references);


Was this same pattern used in another test change? If so, could this be extracted to a helper method?

We will add helpers in the near future after we get byte-for-byte equality working in unit tests.

RikkiGibson · 2021-03-15T19:25:14Z

The rebuild failures repro locally. Will have to figure out why I'm off by 1 byte in some cases after this change.

  Rebuilds with output differences
        C:\Users\rikki\src\roslyn2\artifacts\obj\AnalyzerRunner\Release\net5.0\AnalyzerRunner.dll
        C:\Users\rikki\src\roslyn2\artifacts\obj\AnalyzerRunner\Release\netcoreapp3.1\AnalyzerRunner.dll
        C:\Users\rikki\src\roslyn2\artifacts\obj\AnalyzerRunner\Release\net472\AnalyzerRunner.exe
        C:\Users\rikki\src\roslyn2\artifacts\obj\Microsoft.CodeAnalysis.LanguageServerIndexFormat.Generator\Release\net472\Microsoft.CodeAnalysis.LanguageServerIndexFormat.Generator.exe
        C:\Users\rikki\src\roslyn2\artifacts\obj\RunTests\Release\netcoreapp3.1\RunTests.dll

RikkiGibson · 2021-03-15T22:34:29Z

I found that the change to pass Compilation.GetEntryPoint() as the debugEntryPoint caused specific rebuilds to fail where the project has a Task Main().

Once we get the ability to do byte-for-byte equality in rebuild tests, we should add tests for various entry points, including async and Task-returning Main.

jaredpar · 2021-03-16T03:42:41Z

src/Compilers/Core/Rebuild/CompilationOptionsReader.cs

- : null;
-
- private (string MainTypeName, string MainMethodName)? GetMainMethodInfo()
+ public (string MainTypeName, string MainMethodName)? GetMainMethodInfo()


This is a public method on a public type and hence tuples are generally discourage. Don't have to fix in this PR but feel like this will become a record before we can "ship" this library.

jaredpar · 2021-03-16T03:43:02Z

src/Compilers/Core/Rebuild/CompilationOptionsReader.cs

- : null;
+ // Here we only want to give the caller the main type name if the main method is named "Main" per convention.
+ // If the main method has another name, we have to assume that specifying a main type name won't work.
+ // For example, if the compilation uses top-level statements.


Any reason you didn't make this an XML doc?

I moved this to an inline comment for now. I am fine with it being either inline or XML doc, but it seems like it would be more appropriate on the "root" GetMethodMainInfo if it were moved to XML doc.

It's not currently clear to me that CompilationOptionsReader is going to be part of the public API in the long term. For example, perhaps we will go back to accepting the PEReader+MetadataReader combo directly out of the feeling that we don't need to provide the individual facilities of the CompilationOptionsReader to library consumers.

jaredpar · 2021-03-16T03:43:24Z

src/Compilers/Core/Rebuild/CompilationOptionsReader.cs

@@ -155,6 +152,11 @@ public ImmutableArray<MetadataReferenceInfo> GetMetadataReferences()
 var mdReader = PeReader.GetMetadataReader();
 var methodDefinition = mdReader.GetMethodDefinition(header.EntryPoint);
 var methodName = mdReader.GetString(methodDefinition.Name);
+ if (methodName != WellKnownMemberNames.EntryPointMethodName)


Why return null here vs. adding an extra field to the tuple that says IsTopLevelStatements?

The current approach is in some ways hiding information and it's asking callers to infer behavior based on that information missing. Callers have to know that when this is null then "ah it's top level statements, do another trick". Rather than that why not return all the info and explicitly state this is top level statements case?

In my view, the purpose of the method is to answer the question "what main type name should I pass to the compilation/emit options." If the answer is "none", then it's ok to return null. Would be happy to rename this to try and make that more explicit.

Here is another way of phrasing my question: today null can represent two different outcomes

Inability to read the metadata correctly

Metadata read correctly but this is a program compiled with top level statements

Should consumers react the same way to both situations?

Inability to read the metadata correctly

I think if the tool can detect that the metadata is malformed, it might be good to throw and turn this into a misc error. But if the metadata is just absent, it feels ok for the consumer to do what they were doing before this PR--which is to react to the null by saying "ok, I just won't pass anything for the mainType or the debugEntryPoint."

jaredpar · 2021-03-16T03:46:50Z

src/Compilers/Core/RebuildTest/CSharpRebuildTests.cs

+
+namespace Microsoft.CodeAnalysis.Rebuild.UnitTests
+{
+ public class CSharpRebuildTests : CSharpTestBase


Why the new class here vs. adding onto the existing test case?

I felt like "round tripping various compilation options" was pretty well delineated as a category of tests from "using various C# language features"

What type of test do you imagine won't involve a round trip at the end though? That in many ways is the ultimate test of the code here 😄

They all will test that round tripping works. But some tests are primarily about passing "unusual" compilation options while other tests are primarily about using "unusual" language features.

That said, if you want me to put this in the existing file, please let me know and I will.

I'll let you decide.

jaredpar · 2021-03-16T22:15:48Z

src/Compilers/Core/Rebuild/CompilationOptionsReader.cs

@@ -155,6 +152,11 @@ public ImmutableArray<MetadataReferenceInfo> GetMetadataReferences()
 var mdReader = PeReader.GetMetadataReader();
 var methodDefinition = mdReader.GetMethodDefinition(header.EntryPoint);
 var methodName = mdReader.GetString(methodDefinition.Name);
+ if (methodName != WellKnownMemberNames.EntryPointMethodName)


Here is another way of phrasing my question: today null can represent two different outcomes

Inability to read the metadata correctly

Metadata read correctly but this is a program compiled with top level statements

Should consumers react the same way to both situations?

jaredpar · 2021-03-16T22:17:01Z

src/Compilers/Core/RebuildTest/CSharpRebuildTests.cs

+
+namespace Microsoft.CodeAnalysis.Rebuild.UnitTests
+{
+ public class CSharpRebuildTests : CSharpTestBase


I'll let you decide.

Rebuild compilations that use top-level statements

e093816

RikkiGibson requested review from a team as code owners March 12, 2021 19:34

dotnet-issue-labeler bot added the Area-Compilers label Mar 12, 2021

cston reviewed Mar 12, 2021

View reviewed changes

Fix rebuilding projects with Task Main()

c44eb97

Remove unused usings

a58b666

RikkiGibson mentioned this pull request Mar 15, 2021

Unit test various entry points with Rebuild #51894

Open

cston approved these changes Mar 15, 2021

View reviewed changes

jaredpar reviewed Mar 16, 2021

View reviewed changes

Update CompilationOptionsReader.cs

26e7d3e

jaredpar approved these changes Mar 16, 2021

View reviewed changes

RikkiGibson added 2 commits March 16, 2021 15:37

Merge branch 'main' of github.com:dotnet/roslyn into rebuild-entrypoint

8ab23fe

Fix test

601e582

RikkiGibson merged commit c016e64 into dotnet:main Mar 17, 2021

ghost added this to the Next milestone Mar 17, 2021

RikkiGibson deleted the rebuild-entrypoint branch March 17, 2021 04:46

RikkiGibson mentioned this pull request Mar 17, 2021

Correctly pass debugEntryPoint in Rebuild #51895

Open

allisonchou modified the milestones: Next, 16.10.P2 Mar 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rebuild compilations that use top-level statements #51845

Rebuild compilations that use top-level statements #51845

RikkiGibson commented Mar 12, 2021 •

edited

Loading

cston Mar 12, 2021 •

edited

Loading

cston Mar 12, 2021

RikkiGibson Mar 15, 2021

RikkiGibson commented Mar 15, 2021

RikkiGibson commented Mar 15, 2021

jaredpar Mar 16, 2021

jaredpar Mar 16, 2021

RikkiGibson Mar 16, 2021

jaredpar Mar 16, 2021

RikkiGibson Mar 16, 2021

jaredpar Mar 16, 2021

RikkiGibson Mar 16, 2021

jaredpar Mar 16, 2021

RikkiGibson Mar 16, 2021

jaredpar Mar 16, 2021

RikkiGibson Mar 16, 2021 •

edited

Loading

jaredpar Mar 16, 2021

jaredpar Mar 16, 2021

jaredpar Mar 16, 2021

Rebuild compilations that use top-level statements #51845

Rebuild compilations that use top-level statements #51845

Conversation

RikkiGibson commented Mar 12, 2021 • edited Loading

cston Mar 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RikkiGibson commented Mar 15, 2021

RikkiGibson commented Mar 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RikkiGibson Mar 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RikkiGibson commented Mar 12, 2021 •

edited

Loading

cston Mar 12, 2021 •

edited

Loading

RikkiGibson Mar 16, 2021 •

edited

Loading